Skip to content

Rich error output during baseline establishment (CF-646) #265

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Jun 2, 2025
Merged

Rich error output during baseline establishment (CF-646) #265

merged 8 commits into from
Jun 2, 2025

Conversation

@aseembits93
Copy link
Contributor

@aseembits93 aseembits93 commented Jun 2, 2025 •
edited
Loading

User description

It could help the user with cancelling the current run if certain modules are not found during a run.

this PR will only show the first import error as the code would stop executing the moment the first import fails. capturing all import errors will require a different way of test instrumentation.

PR Type

Enhancement

Description

  • Improve error reporting on missing modules

  • Detect ModuleNotFoundError in test outputs

  • Show errors using Rich panel

  • Add import for regex parsing

Changes walkthrough 📝

Relevant files
Error handling
function_optimizer.py
Detect and display ModuleNotFoundError                                     

codeflash/optimization/function_optimizer.py

  • Added import of re for error parsing
  • Check stdout for 'ModuleNotFoundError'
  • Use Rich Text and Panel for styled output
  • Console.print error panel on failure
    		• +10/-0   
    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
    •

    @github-actions
    Copy link

    github-actions bot commented Jun 2, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Missing Import

    The code uses Panel and console without importing them, which will cause a NameError at runtime. Ensure you import Panel (e.g., from rich.panel) and initialize or import a console instance before using them.

     
        if 'ModuleNotFoundError' in run_result.stdout:
        from rich.text import Text
        match = re.search(r'^.*ModuleNotFoundError.*$', run_result.stdout, re.MULTILINE).group()
        panel = Panel(
            Text.from_markup(f"⚠️  {match} ", style="bold red"),
            expand=False,
        )
        console.print(panel)
if testing_type in {TestingMode.BEHAVIOR, TestingMode.PERFORMANCE}:
    Unsafe Regex Handling

    Calling .group() on the result of re.search without checking if a match was found can lead to an AttributeError. Add a null check on the match object before using group().

     
    match = re.search(r'^.*ModuleNotFoundError.*$', run_result.stdout, re.MULTILINE).group()

    @github-actions
    Copy link

    github-actions bot commented Jun 2, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Check stderr for errors

    Include run_result.stderr in the error check so that module not found messages
    emitted to stderr are also captured.

    codeflash/optimization/function_optimizer.py [1197]

    -if 'ModuleNotFoundError' in run_result.stdout:
+combined_output = run_result.stdout + run_result.stderr
+if 'ModuleNotFoundError' in combined_output:
    Suggestion importance[1-10]: 6

    __

    Why: Including stderr in the search for ModuleNotFoundError captures errors emitted there as well, improving robustness of the check.

    		Low
    Possible issue
    Guard regex group call

    Add a check that the regex search succeeded before calling .group() to prevent an
    AttributeError if no match is found.

    codeflash/optimization/function_optimizer.py [1199]

    -match = re.search(r'^.*ModuleNotFoundError.*$', run_result.stdout, re.MULTILINE).group()
+match_obj = re.search(r'^.*ModuleNotFoundError.*$', run_result.stdout, re.MULTILINE)
+if match_obj:
+    match = match_obj.group()
+else:
+    match = "ModuleNotFoundError occurred"
    Suggestion importance[1-10]: 4

    __

    Why: Guarding the .group() call prevents an AttributeError if the regex unexpectedly finds no match, although the preceding substring check makes failure unlikely.

    		Low

    @aseembits93 aseembits93 requested a review from KRRT7 June 2, 2025 21:58
    @aseembits93 aseembits93 changed the title Rich error output during baseline establishment Rich error output during baseline establishment (CF-646) Jun 2, 2025
    KRRT7
    KRRT7 requested changes Jun 2, 2025
    View reviewed changes
    Copy link
    Contributor

    @KRRT7 KRRT7 left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    place the regex in a common place and then import it within the test, if the regex every changes then the test will automatically pick the change

    @aseembits93 aseembits93 requested a review from KRRT7 June 2, 2025 22:30
    KRRT7
    KRRT7 reviewed Jun 2, 2025
    View reviewed changes
    KRRT7
    KRRT7 approved these changes Jun 2, 2025
    View reviewed changes
    Copy link
    Contributor

    @KRRT7 KRRT7 left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    LGTM!

    @openhands-ai
    Copy link

    openhands-ai bot commented Jun 2, 2025

    Looks like there are a few issues preventing this PR from being merged!

    • GitHub Actions are failing:
      • end-to-end-test

    If you'd like me to help, just leave a comment, like

    @OpenHands please fix the failing actions on PR #265

    Feel free to include any additional details that might help me get this PR into a better state.

    You can manage your notification settings

    @aseembits93 aseembits93 merged commit 47f6c02 into main Jun 2, 2025
    16 of 17 checks passed
    @aseembits93 aseembits93 deleted the error-output-nonverbose branch June 2, 2025 23:44
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @KRRT7 KRRT7 KRRT7 approved these changes

    Assignees

    No one assigned

    Labels

    Review effort 2/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    3 participants

    @aseembits93 @KRRT7